Skip to content

fix(mcp): do not pass all headers to upstream#2345

Merged
StarpTech merged 2 commits intomainfrom
dustin/dont-forward-hop-headers
Nov 19, 2025
Merged

fix(mcp): do not pass all headers to upstream#2345
StarpTech merged 2 commits intomainfrom
dustin/dont-forward-hop-headers

Conversation

@StarpTech
Copy link
Copy Markdown
Contributor

@StarpTech StarpTech commented Nov 19, 2025 •
edited by coderabbitai Bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced header filtering to prevent hop-by-hop headers (Proxy-Authenticate, Proxy-Authorization, Alt-Svc, Proxy-Connection) from being forwarded to downstream GraphQL servers, while ensuring authorized headers are properly transmitted. Improved error handling for missing request headers.✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

@StarpTech StarpTech requested a review from asoorm November 19, 2025 19:44
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 19, 2025 •
edited
Loading

Walkthrough

The PR adds filtering logic to prevent specific hop-by-hop and filtered headers (e.g., Proxy-Authenticate, Proxy-Authorization, Alt-Svc, Proxy-Connection) from being forwarded to downstream GraphQL servers during request execution, along with test coverage verifying the filtering behavior.

Changes

Cohort / File(s) Summary
Header forwarding filtering logic
router/pkg/mcpserver/server.go		 Introduces a set of headers to skip during request forwarding, adds filtering logic in GraphQL query execution to exclude these headers, and enhances error handling for missing request headers extraction from context.
Header filtering test verification
router-tests/mcp_test.go		 Adds a new subtest validating that hop-by-hop headers are not forwarded while permitted headers remain forwarded, verifying Content-Type, Accept, and Accept-Encoding propagation across the forwarding chain.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The header filtering implementation is straightforward—defining a skip set and applying it during forwarding
  • Test addition follows existing patterns and verifies expected behavior clearly
  • Changes are localized to a specific concern (header forwarding)

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing hop-by-hop and filtered headers from being forwarded to the upstream GraphQL server, which is the core objective of this PR.
✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dde0ec0 and 67d053b.

📒 Files selected for processing (2)
  • router-tests/mcp_test.go (1 hunks)
  • router/pkg/mcpserver/server.go (2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings 
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
📚 Learning: 2025-09-17T20:55:39.456Z 
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-28T09:18:10.121Z 
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-20T22:13:25.222Z 
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-10-01T20:39:16.113Z 
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-20T10:08:17.857Z 
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-09-01T13:44:13.045Z 
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.045Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false as the first check, so no external nil checks are needed when calling c.retryOptions.IsEnabled(). This is a consistent pattern used across multiple IsEnabled() methods in the codebase.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-20T10:08:17.857Z 
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-09-01T13:44:13.045Z 
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.045Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false, so no external nil checks are needed when calling c.retryOptions.IsEnabled().

Applied to files:

  • router-tests/mcp_test.go
🧬 Code graph analysis (1)
router-tests/mcp_test.go (3)
router-tests/testenv/testenv.go (4)
  • Run (105-122)
  • Config (285-342)
  • SubgraphsConfig (367-380)
  • Environment (1733-1769)
router/pkg/config/config.go (6)
  • Config (1010-1085)
  • MCPConfiguration (970-980)
  • HeaderRules (254-259)
  • GlobalHeaderRule (261-265)
  • RequestHeaderRule (279-302)
  • HeaderRuleOperationPropagate (270-270)
router/core/router.go (2)
  • Option (172-172)
  • WithHeaderRules (1730-1734)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: build-router
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./events)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: image_scan
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/pkg/mcpserver/server.go (2)

40-61: LGTM! Well-designed header filtering list.

The skippedHeaders map correctly includes:

  • RFC 7230 hop-by-hop headers (Connection, Keep-Alive, Te, Trailer, Transfer-Encoding, Upgrade)
  • Proxy-related headers for security
  • Request-specific headers that are explicitly set later (Content-Type, Accept at lines 723-724)
  • WebSocket headers that don't apply to GraphQL requests

All keys are in canonical form, which is correct for use with http.Header.

710-720: LGTM! Efficient header filtering implementation.

The filtering logic correctly:

  1. Iterates through headers from the context
  2. Skips headers present in the skippedHeaders map (lines 712-715)
  3. Forwards all other headers to the GraphQL request

The explicit setting of Accept and Content-Type headers at lines 723-724 after the filtering loop ensures these critical headers have the correct values for GraphQL requests, regardless of what the client sent.

router-tests/mcp_test.go (1)

878-983: LGTM! Comprehensive test coverage for header filtering.

This test effectively validates the header filtering implementation by:

  1. Configuring wildcard header forwarding (.*) to prove the MCP server filters before the router
  2. Sending multiple categories of headers that should be filtered (hop-by-hop, proxy, content-negotiation)
  3. Including a control header (X-Allowed-Header) to verify non-filtered headers are forwarded
  4. Asserting that filtered headers are absent or replaced with correct values
  5. Verifying Content-Type and Accept-Encoding are set appropriately by the MCP server and Go HTTP client

The test follows the existing pattern established at line 760 and provides end-to-end validation of the filtering behavior.

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 19, 2025 •
edited
Loading

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-67f36459054b4770d92333b2c0fd2d01a7294393

@StarpTech StarpTech merged commit d508fa1 into main Nov 19, 2025
38 of 39 checks passed
@StarpTech StarpTech deleted the dustin/dont-forward-hop-headers branch November 19, 2025 20:15
@coderabbitai coderabbitai Bot mentioned this pull request Dec 3, 2025
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jan 31, 2026
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@endigma endigma endigma approved these changes

@asoorm asoorm Awaiting requested review from asoorm

@Noroth Noroth Awaiting requested review from Noroth Noroth is a code owner

@devsergiy devsergiy Awaiting requested review from devsergiy devsergiy is a code owner

@jensneuse jensneuse Awaiting requested review from jensneuse jensneuse is a code owner

Assignees

No one assigned

Labels

router

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@StarpTech @endigma